Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Throws check tick all over the photo code #4584

Closed
wants to merge 2 commits into from

Conversation

morrowwolf
Copy link
Member

@morrowwolf morrowwolf commented Oct 4, 2023

About the pull request

So I have a general understanding of how this works in scheduling and all but I'm sure this could be done better.

This should help with the absolutely gnarly 3-4 second hard stops on the server when cameras are taking pictures of lots of stuff at 7 width.

Explain why it's good for the game

Hard server freezes are bad (hypothetically)

Testing Photographs and Procedure

Screenshots & Videos

Put screenshots and videos here with an empty line between the screenshots and the <details> tags.

Changelog

🆑 Morrow
fix: Throws check tick all over the photo code to stop hard freezes
/:cl:

@github-actions github-actions bot added the Fix Fix one bug, make ten more label Oct 4, 2023
@@ -184,6 +184,7 @@
for(var/atom/A in the_turf)
if(A.invisibility) continue
atoms.Add(A)
CHECK_TICK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking tick on just loops filtering out contents I'm skeptical is helpful.

@@ -262,6 +267,7 @@
var/list/turf/turfs = RANGE_TURFS(radius, target) & view(world_view_size + radius, user.client)
for(var/turf/T as anything in turfs)
mobs += get_mobs(T)
CHECK_TICK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checking tick on just loops filtering out contents I'm skeptical is helpful.

@@ -218,6 +220,7 @@
var/image/IM = getFlatIcon(the_turf.loc)
if(IM)
res.Blend(IM, blendMode2iconMode(the_turf.blend_mode),xoff,yoff)
Copy link
Contributor

@Drulikar Drulikar Oct 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
res.Blend(IM, blendMode2iconMode(the_turf.blend_mode),xoff,yoff)
CHECK_TICK
res.Blend(IM, blendMode2iconMode(the_turf.blend_mode),xoff,yoff)

The get flat icon just before this did a bunch of blends so worth checking tick before this blend.

Also try testing this by merging with master to get the getflaticon fixes pr in and you can better evaluate if this resolves any issues with large pictures (you may need to opt in to the more expensive get flat icon only for testing - not something the cameras should generally opt into unless we find cases where the extra work is needed for photos)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also whitespace may be funky on mobile - not sure why this suggestion diff is red lining the existing code - just want a new line with check tick.

@@ -209,6 +210,7 @@
xoff+=A:step_x
yoff+=A:step_y
res.Blend(IM, blendMode2iconMode(A.blend_mode), A.pixel_x + xoff, A.pixel_y + yoff)
CHECK_TICK
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Harry when he sees single letter vars in the code context 👀

@morrowwolf morrowwolf marked this pull request as draft October 5, 2023 05:38
@morrowwolf morrowwolf closed this Oct 5, 2023
@Drulikar Drulikar mentioned this pull request Oct 11, 2023
github-merge-queue bot pushed a commit that referenced this pull request Oct 12, 2023
# About the pull request

This PR is a partial revival of #4584 in addition to just a general
refactor of the code it was affecting. Now after every blend or
getFlatIcon call there is a CHECK_TICK. Various loops use the `as
anything` usage as possible to help with type checking. Additionally,
the descriptions for mobs is a little more fluid, and now there is
actually a description for a photo with no mobs in it.

# Explain why it's good for the game

This should hopefully mitigate issues where photos cause the server to
stutter, but hard to know for sure without testing on live. Also the
description was annoyingly rigid.

# Testing Photographs and Procedure
<details>
<summary>Screenshots & Videos</summary>


![image](https://github.com/cmss13-devs/cmss13/assets/76988376/4d3d5dc4-bd1e-4a63-a188-b0a21076ee99)
(There was an additional space in this photo's description that was
since removed)

</details>

# Changelog
:cl: Drathek
refactor: Refactored camera code to be less blocking, use typechecks
less often, and provide somewhat more fluid descriptions to photos.
/:cl:

---------

Co-authored-by: harryob <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Fix one bug, make ten more
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants